Ensure sequential execution of connection state callbacks #656
Conversation
sukunrt
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #656 +/- ##
==========================================
+ Coverage 78.84% 78.89% +0.04%
==========================================
Files 42 42
Lines 4444 4454 +10
==========================================
+ Hits 3504 3514 +10
Misses 722 722
Partials 218 218
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
agent.go
Outdated
| @@ -278,7 +278,7 @@ func NewAgent(config *AgentConfig) (*Agent, error) { //nolint:gocognit | |||
|
|
|||
| a := &Agent{ | |||
| chanTask: make(chan task), | |||
| chanState: make(chan ConnectionState), | |||
| chanState: make(chan ConnectionState, 4), | |||
There was a problem hiding this comment.
Why 4 ?
Won't this deadlock if we get unlucky and there is 5 events queued up.
There was a problem hiding this comment.
We only need this to close the agent correctly. The current behaviour allows agent closing in the callback which with a channel capacity of 0 would lead to deadlock.
I will see what the right way to handle this is.
There was a problem hiding this comment.
Considering that we can go Connected to Disconnected to Connected many many times, I don't think a limit to the channel can work. Especially considering that we don't want to block the go routine sending to chanState. So I've introduced a queue here which runs the callbacks in a separate goroutine
sukunrt
changed the title
sukunrt
force-pushed
the
update-state-seq
branch
3 times, most recently
from
a069da8
to
dbc776c
Compare
sukunrt
requested review from
Jorropo and
Sean-Der
and removed request for
Jorropo
Before we launched a goroutine to announce every ConnectionState change to users. These could then be sent to the user out of order. This commit adds a connectionStateNotifier. The connectionStateNotifier delivers them sequentially to the user. Resolves #624
Sean-Der
force-pushed
the
update-state-seq
branch
from
dbc776c
to
59cc806
Compare
Ice update connection state callbacks need to happen sequentially otherwise a later callback could overwrite a newer callback.
Fixes: #624